OCPP: serialize dateTime with fractional seconds#31191
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Overriding
types.DateTimeFormatin aninit()function introduces a hidden global side effect; consider either exposing an explicit initializer or wiring this through configuration so consumers of thetypespackage are aware that the format is being changed globally.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Overriding `types.DateTimeFormat` in an `init()` function introduces a hidden global side effect; consider either exposing an explicit initializer or wiring this through configuration so consumers of the `types` package are aware that the format is being changed globally.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Overriding the global
types.DateTimeFormatininitcan be surprising for other OCPP usages in the same process; consider either scoping this behavior behind a configuration flag or at least documenting this side effect clearly whereConfigor the package is consumed. - Instead of a custom
dateTimeFormatstring, consider usingtime.RFC3339Nanoand then constraining precision (e.g., truncating to milliseconds) at thetime.Timelevel to rely on the standard library’s formatting guarantees and avoid drift if RFC3339 handling evolves.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Overriding the global `types.DateTimeFormat` in `init` can be surprising for other OCPP usages in the same process; consider either scoping this behavior behind a configuration flag or at least documenting this side effect clearly where `Config` or the package is consumed.
- Instead of a custom `dateTimeFormat` string, consider using `time.RFC3339Nano` and then constraining precision (e.g., truncating to milliseconds) at the `time.Time` level to rely on the standard library’s formatting guarantees and avoid drift if RFC3339 handling evolves.
## Individual Comments
### Comment 1
<location path="charger/ocpp/instance_test.go" line_range="41-43" />
<code_context>
+ require.Equal(t, dateTimeFormat, types.DateTimeFormat)
+
+ ts := types.NewDateTime(time.Date(2026, 6, 23, 12, 45, 9, 404_000_000, time.UTC))
+ b, err := json.Marshal(ts)
+ require.NoError(t, err)
+ require.JSONEq(t, `"2026-06-23T12:45:09.404Z"`, string(b))
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add a round-trip (marshal + unmarshal) test to ensure deserialization remains compatible
Right now we only assert the marshalled JSON string. Please also unmarshal that JSON back into `types.DateTime` (or the relevant type) and assert it matches the original time (accounting for location if needed) so we verify decode as well as encode with fractional seconds.
Suggested implementation:
```golang
func TestMain(m *testing.M) {
}
}
}
func TestDateTimeFormatHasFractionalSeconds(t *testing.T) {
require.Equal(t, dateTimeFormat, types.DateTimeFormat)
original := types.NewDateTime(time.Date(2026, 6, 23, 12, 45, 9, 404_000_000, time.UTC))
// Marshal to JSON and verify the wire format (including fractional seconds)
b, err := json.Marshal(original)
require.NoError(t, err)
require.JSONEq(t, `"2026-06-23T12:45:09.404Z"`, string(b))
// Unmarshal back and ensure we get the same instant in time
var roundTripped types.DateTime
err = json.Unmarshal(b, &roundTripped)
require.NoError(t, err)
// Compare underlying time values to verify round-trip correctness
require.True(t, time.Time(original).Equal(time.Time(roundTripped)))
}
```
The assertion `time.Time(original).Equal(time.Time(roundTripped))` assumes that `types.DateTime` is defined as `type DateTime time.Time`. If the actual definition differs (for example, if it is a struct wrapper or exposes a method to retrieve the underlying `time.Time`), adjust the comparison accordingly, e.g. `original.Time().Equal(roundTripped.Time())` or another equivalent accessor.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| b, err := json.Marshal(ts) | ||
| require.NoError(t, err) | ||
| require.JSONEq(t, `"2026-06-23T12:45:09.404Z"`, string(b)) |
There was a problem hiding this comment.
suggestion (testing): Add a round-trip (marshal + unmarshal) test to ensure deserialization remains compatible
Right now we only assert the marshalled JSON string. Please also unmarshal that JSON back into types.DateTime (or the relevant type) and assert it matches the original time (accounting for location if needed) so we verify decode as well as encode with fractional seconds.
Suggested implementation:
func TestMain(m *testing.M) {
}
}
}
func TestDateTimeFormatHasFractionalSeconds(t *testing.T) {
require.Equal(t, dateTimeFormat, types.DateTimeFormat)
original := types.NewDateTime(time.Date(2026, 6, 23, 12, 45, 9, 404_000_000, time.UTC))
// Marshal to JSON and verify the wire format (including fractional seconds)
b, err := json.Marshal(original)
require.NoError(t, err)
require.JSONEq(t, `"2026-06-23T12:45:09.404Z"`, string(b))
// Unmarshal back and ensure we get the same instant in time
var roundTripped types.DateTime
err = json.Unmarshal(b, &roundTripped)
require.NoError(t, err)
// Compare underlying time values to verify round-trip correctness
require.True(t, time.Time(original).Equal(time.Time(roundTripped)))
}The assertion time.Time(original).Equal(time.Time(roundTripped)) assumes that types.DateTime is defined as type DateTime time.Time. If the actual definition differs (for example, if it is a struct wrapper or exposes a method to retrieve the underlying time.Time), adjust the comparison accordingly, e.g. original.Time().Equal(roundTripped.Time()) or another equivalent accessor.
|
Afaiu OCPP uses ISO8106 (or seomthing like this) which is much more lose. Implication: RFC3339 without fractional seconds is perfectly valid. We can try this but I‘m convinced that it will just break the next charger in no time and we’ll have to revert it. What the library doesn‘t allow is format relaxations per charger. In any case, users should open issues with charger vendors. |
Changes the OCPP
dateTimeserialisation format to RFC3339 with fractional seconds (2026-06-23T12:45:09.404Z).Previously, ocpp-go used the default
time.RFC3339(second granularity, no fractional part).Background: Some chargers (e.g. Webasto/Ampure NEXT) reject timestamps without a fractional part.
Fixes part of #31158